Skip to content

Fix home-page flicker AND post-login non-interactive page (#725)#1318

Merged
milanmajchrak merged 1 commit into
customer/vsb-tuofrom
fix/flickering-and-slow-login
Jun 22, 2026
Merged

Fix home-page flicker AND post-login non-interactive page (#725)#1318
milanmajchrak merged 1 commit into
customer/vsb-tuofrom
fix/flickering-and-slow-login

Conversation

@milanmajchrak

@milanmajchrak milanmajchrak commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Problem

Issue dspace-customers#725 and the tug-of-war between PR #1288 and PR #1317:

Root cause — why #1288 and #1317 conflict

This FE is Angular 15 + ngUniversal with no hydration (provideClientHydration is Angular 16+), so every browser load tears down the SSR DOM and re-renders from scratch. PR #1288 masks that rebuild with a snapshot overlay (#__dspace_ssr_overlay, pointer-events:none) over a hidden <ds-app>, removed via window.__dspaceRemoveSsrOverlay(). Both PRs edit the same decision: when to call that removal, and each picks a signal at the opposite end of one tradeoff:

PR Removal trigger Result
#1288 ApplicationRef.isStable Content always painted ⇒ no flicker. But isStable is held hostage by any ongoing zone async — after an admin login (authz/widgets, polling, AAI scripts) it fires many seconds late or hits the 15 s fallback ⇒ page masked & non-interactive (#725).
#1317 !isAuthenticationBlocking && !isThemeLoading + 1×requestAnimationFrame Fires promptly ⇒ fixes #725. But that gate only un-hides <router-outlet>; Angular hasn't rendered the routed page yet and one rAF runs before paint ⇒ snapshot dropped over an empty <ds-app>flicker returns.

Neither signal means "the routed content for this page is committed to the DOM and painted, and the app is interactive." isStable over-waits (couples removal to unrelated background work); the auth/theme gate under-waits (decoupled from actual content paint). So fixing one re-breaks the other.

The fix (src/app/app.component.ts)

Keep #1317's decoupling from isStable (so background async can never delay us), but after the gate opens, wait across animation frames until the real <ds-app> is actually laid out (height ≥ 200px and its #main-content host present — i.e. no longer the empty shell the overlay script left) before dropping the snapshot, one frame later. Capped at MAX_FRAMES (~3 s) so nothing can hold the overlay open; the 15 s hard fallback in index.html stays as the catastrophic-error net.

This is deterministic, not a favourable race: removal happens only after contentPainted() is true, so the real <ds-app> is never empty when the snapshot drops.

Verification (DSpace 7.6.5 backend, Playwright + Chromium, CPU throttled 4×, hard reload / ctrl+shift+R)

Measured on one performance.now() timeline. overlayRemoved ≈ time-to-interactive (overlay is pointer-events:none over a hidden <ds-app>). ds-app height @ removal = 0 ⇒ blank flash.

Scenario Code TTI content painted ds-app height @ removal Verdict
admin reload #1288 15963 ms 2947 ms 5281 px SLOW: ~13 s masked (#725)
admin reload #1317 3797 ms 4014 ms 0 px FLICKER: 217 ms blank
admin reload this fix 3064–3421 ms 3068 ms 5281 px NO-FLICKER + interactive ~3.4 s
anon reload this fix 2911 ms 2357 ms 1461 px NO-FLICKER

Both after-fix runs: 0 CORS errors and 0 SSR/hydration/NG0/ExpressionChanged console errors. Admin run repeated ×3 — all NO-FLICKER (height 5281 px, gap ≤ 0).

"No flicker", measurably: at the instant the overlay is removed the real <ds-app> is not empty (height ≥ 200px with #main-content), so the blank window max(0, contentReady − overlayRemoved) is ≤ 0 ms.

Verification videos

Hosted on the flicker-fix-evidence branch (kept off this PR's diff). Click to play in GitHub's in-browser player, or download:

Before

After (this fix)

Notes

  • Existing AppComponent specs (removeSsrOverlayWhenContentVisible) are unchanged and remain valid (synchronous-rAF shim + frame cap ⇒ removal still fires once). The new content-paint wait is verified via the Playwright runs above. The full karma suite was not run locally; CI covers it.
  • The viewevents / google.analytics console entries seen during measurement are the throwaway test backend lacking statistics/analytics config — not SSR/hydration errors and not present on VSB.

Refs: dspace-customers#725, PR #1288, PR #1317

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f7bda491-859c-4b9c-acc1-1db00748d4cb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

… on isStable or bare auth gate)

The SSR anti-flicker overlay (PR #1288) is removed by AppComponent. Two prior
approaches each fixed one symptom and reintroduced the other:

- #1288 waited for ApplicationRef.isStable: no flicker, but admin sessions keep
  the zone busy (authz/widgets, polling, AAI scripts) so isStable fires many
  seconds late (or hits the 15s fallback), leaving the live, already-rendered page
  masked & non-interactive (dspace-customers#725).
- #1317 switched to the loader-swap gate (!isAuthenticationBlocking &&
  !isThemeLoading) + a single requestAnimationFrame: fast/interactive, but the
  gate only un-hides <router-outlet> and one rAF runs before paint, so the
  snapshot is dropped over an empty <ds-app> -> the flicker returned.

Neither signal means "the routed content is painted AND the app is interactive":
isStable over-waits (couples removal to unrelated background async); the auth/theme
gate under-waits (decoupled from actual content paint).

This keeps #1317's decoupling from isStable but, after the gate opens, waits across
animation frames until the real <ds-app> is actually laid out (height >= 200px AND
its #main-content host present) before removing the overlay, capped at ~3s
(MAX_FRAMES) so background async can never hold it open. The 15s fallback in
index.html stays as the catastrophic-error net.

Verified (DSpace 7.6.5 backend, CPU 4x, hard reload, admin session):
- #1288: TTI 15963ms (page masked ~13s)  #1317: ds-app height 0 at removal (217ms flash)
- fix: TTI ~3.1-3.4s, ds-app height 5281px at removal, gap <= 0 (no flash), 3x deterministic;
  anon reload also no-flicker; 0 CORS and 0 SSR/hydration/NG0 console errors.
Verification videos are linked in the PR description.

Refs: dspace-customers#725, PR #1288, PR #1317

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@milanmajchrak milanmajchrak force-pushed the fix/flickering-and-slow-login branch from 1a23f47 to 43fb507 Compare June 22, 2026 07:19
@milanmajchrak milanmajchrak merged commit dce52f0 into customer/vsb-tuo Jun 22, 2026
6 checks passed
milanmajchrak added a commit that referenced this pull request Jun 23, 2026
…er WebDriver (close #725)

Addresses an expert review of the DOM-settle change: DOM-settle alone shortened but did not
eliminate issue #725, because the masking window still left the page non-interactive (the opaque
overlay sat over a visibility:hidden <ds-app>, so clicks landed on nothing) and still produced
duplicate DOM that breaks strict-mode E2E locators.

Changes:
- index.html: drop the `ds-app[data-dspace-ssr-hidden]{visibility:hidden}` rule and give the overlay
  min-height:100vh. The overlay is already opaque + pointer-events:none, so it still masks the CSR
  rebuild visually, but clicks now pass THROUGH it to the real, still-visible <ds-app> underneath.
  The page is therefore interactive the entire time it is masked -> #725 ("looks rendered but
  nothing is clickable") cannot recur, even if removal rides the 10s cap. The snapshot is marked
  aria-hidden so AT (and the duplicate-node concern) target the live app, not the snapshot.
- index.html: also bypass the overlay for WebDriver runners (navigator.webdriver), mirroring the
  existing Cypress guard, so Playwright/Selenium see no overlay and no duplicate DOM (fixes the
  #725 strict-mode locator failure).
- app.component.ts: exclude the admin-sidebar subtree from the DOM-settle MutationObserver (its long
  :enter/:leave animations would keep re-arming the quiet window and push admin logins toward the
  cap); add OnDestroy + takeUntil + observer/timer teardown.
- Fix stale `ApplicationRef.isStable` / `removeSsrOverlayWhenStable` comments (index.html, typings.d.ts).

Verified against the live build (Playwright, CPU/network throttled):
- WebDriver run: overlay absent (no duplicate DOM).
- webdriver spoofed false (real-user path): a navbar click WHILE the snapshot is still shown reaches
  the live <ds-app> (interactive under mask).
- anon reveal settled, CLS after reveal = 0; admin reveal settled (reason "settled", not cap),
  CLS after reveal = 0.

Refs: dspace-customers#725, PR #1288, PR #1317, PR #1318, PR #1321

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
milanmajchrak added a commit that referenced this pull request Jun 23, 2026
…moved only after the routed DOM settles (#1321)

* fix(ssr-overlay): remove overlay only after the routed page DOM settles (real home-page flicker)

Follow-up to #1318. On a real instance (VSB / dev-6.pc), an incognito Ctrl+Shift+R
still flickered: the deployed code dropped the SSR snapshot ~3.2s in, while the home
page was only half-rendered (search box, community list and several navbar items not
yet present, content showing "Recent Submissions") and then everything popped into
place ~600ms later. Captured frame-by-frame against the live instance.

Root cause: the home page renders piecewise (each section fetches its own data), so the
previous "<ds-app> has #main-content and height>=200" heuristic was satisfied while the
page was still building -> snapshot removed too early -> visible flicker.

Fix: after the auth/theme gate opens, keep the snapshot until the real <ds-app> subtree
has SETTLED -- no element added/removed for SETTLE_QUIET_MS (600ms) -- via a
MutationObserver, requiring minimum content first and capped at SETTLE_MAX_MS (10s). This
stays decoupled from ApplicationRef.isStable (DOM-settle ignores non-rendering background
async), so it does not bring back the post-login non-interactive page (#725): admin
reveals in ~5s, not ~15s.

Validated against the live dev-6.pc instance by intercepting the overlay-removal and
driving it with this condition:
- anon reload : drops @ ~4.8s, page COMPLETE (search + community list + full navbar)
- admin reload: drops @ ~5.0s (reason "settled", not the cap), page complete
vs the deployed code dropping @ ~3.2-3.6s on a half-built page.

Refs: dspace-customers#725, PR #1288, PR #1317, PR #1318

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(ssr-overlay): make the masked page interactive + skip overlay under WebDriver (close #725)

Addresses an expert review of the DOM-settle change: DOM-settle alone shortened but did not
eliminate issue #725, because the masking window still left the page non-interactive (the opaque
overlay sat over a visibility:hidden <ds-app>, so clicks landed on nothing) and still produced
duplicate DOM that breaks strict-mode E2E locators.

Changes:
- index.html: drop the `ds-app[data-dspace-ssr-hidden]{visibility:hidden}` rule and give the overlay
  min-height:100vh. The overlay is already opaque + pointer-events:none, so it still masks the CSR
  rebuild visually, but clicks now pass THROUGH it to the real, still-visible <ds-app> underneath.
  The page is therefore interactive the entire time it is masked -> #725 ("looks rendered but
  nothing is clickable") cannot recur, even if removal rides the 10s cap. The snapshot is marked
  aria-hidden so AT (and the duplicate-node concern) target the live app, not the snapshot.
- index.html: also bypass the overlay for WebDriver runners (navigator.webdriver), mirroring the
  existing Cypress guard, so Playwright/Selenium see no overlay and no duplicate DOM (fixes the
  #725 strict-mode locator failure).
- app.component.ts: exclude the admin-sidebar subtree from the DOM-settle MutationObserver (its long
  :enter/:leave animations would keep re-arming the quiet window and push admin logins toward the
  cap); add OnDestroy + takeUntil + observer/timer teardown.
- Fix stale `ApplicationRef.isStable` / `removeSsrOverlayWhenStable` comments (index.html, typings.d.ts).

Verified against the live build (Playwright, CPU/network throttled):
- WebDriver run: overlay absent (no duplicate DOM).
- webdriver spoofed false (real-user path): a navbar click WHILE the snapshot is still shown reaches
  the live <ds-app> (interactive under mask).
- anon reveal settled, CLS after reveal = 0; admin reveal settled (reason "settled", not cap),
  CLS after reveal = 0.

Refs: dspace-customers#725, PR #1288, PR #1317, PR #1318, PR #1321

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* refactor(ssr-overlay): declarative DOM-settle, drop fragile admin-sidebar hack (no behaviour change)

The overlay-removal logic was a hard-to-read imperative blob (mutable done/quietTimer/capTimer flags,
manual arm/re-arm, manual MutationObserver/teardown bookkeeping) plus a brittle string-selector hack
(`inAdminSidebar` -> `closest('ds-themed-admin-sidebar, ds-admin-sidebar')`).

Rewritten as small, named, single-responsibility pieces:
- removeSsrOverlayWhenContentVisible(): guard + runOutsideAngular + subscribe(takeUntil(destroyed$)).
- routedPageReadyToReveal$(): loader gate (take 1) -> switchMap(dsAppDomSettled$).
- dsAppDomSettled$(): MutationObserver wrapped as an Observable; settle = startWith + debounceTime
  (the quiet window) + filter(real content); race() with timer() for the cap; take(1).
- dsAppHasRenderedContent(), isElementChildListChange(), runAfterNextFrame(): tiny pure helpers.

RxJS now owns debounce, the cap, and teardown (the Observable disconnects the observer on unsubscribe,
takeUntil(destroyed$) ends everything on destroy), so the mutable flags, manual timers and the separate
cancelOverlaySettle field are gone (net -45 lines).

Dropped the admin-sidebar exclusion entirely: it was a fragile, theme-coupled selector guarding a
problem that interactive-under-mask already makes harmless (riding the cap is fine when the page is
clickable throughout) and that does not occur in practice (admin still settles via "settled", not the
cap). Tuning constants moved to named readonly fields.

Behaviour re-verified on the live build (Playwright, throttled): interactive-under-mask still works;
anon + admin both reveal with reason "settled" (not cap) and CLS after reveal = 0.

Refs: dspace-customers#725, PR #1321

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant